-
Notifications
You must be signed in to change notification settings - Fork 72
[LG-5504] feat(input-box): add InputBoxContext
#3290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LG-5504] feat(input-box): add InputBoxContext
#3290
Conversation
…associated types and tests
|
InputBoxContext InputBoxContext
|
Size Change: +216 B (+0.01%) Total Size: 1.61 MB
ℹ️ View Unchanged
|
| return ( | ||
| segment: SegmentEnum[keyof SegmentEnum], | ||
| value: string, | ||
| allowZero?: boolean, | ||
| ): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could benefit from an object arg
|
|
||
| import { ExplicitSegmentRule } from '../utils'; | ||
|
|
||
| export const SegmentObjMock = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if this may also benefit from time mocks in addition to the date mocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I add time mocks, I think it would make more sense to add it in #3285, so I can adjust the current tests.
| @@ -0,0 +1 @@ | |||
| export { type InputSegmentChangeEventHandler } from './InputSegment.types'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will create a circular import between InputBoxContext and InputSegment. This could be addressed by having a shared.types.ts file for types like this change event handler type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be here; it was added by accident.
| charsPerSegment: Record<Segment, number>; | ||
| disabled: boolean; | ||
| segmentEnum: SegmentEnumObject<Segment>; | ||
| onChange: InputSegmentChangeEventHandler<Segment, string>; | ||
| onBlur: (event: React.FocusEvent<HTMLInputElement>) => void; | ||
| segmentRefs: Record<Segment, ReturnType<DynamicRefGetter<HTMLInputElement>>>; | ||
| segments: Record<Segment, string>; | ||
| labelledBy?: string; | ||
| size: Size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If any of these will be exposed by InputBox, they can also be shared in the shared.types.ts file
Any tsdocs worth adding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, these can be shared
| expect(result.current.charsPerSegment).toBe(charsPerSegmentMock); | ||
| expect(result.current.segmentEnum).toBe(SegmentObjMock); | ||
| expect(result.current.onChange).toBe(mockOnChange); | ||
| expect(result.current.onBlur).toBe(mockOnBlur); | ||
| expect(result.current.segmentRefs).toBe(segmentRefsMock); | ||
| expect(result.current.segments).toBe(segmentsMock); | ||
| expect(result.current.size).toBe(Size.Default); | ||
| expect(result.current.disabled).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could destructure to DRY this up
…se object parameter format for improved clarity and consistency
…ity by destructuring context values
| } from './InputBoxContext.types'; | ||
|
|
||
| // The Context constant is defined with the default/fixed type, which is string. This is the loose type because we don't know the type of the string yet. | ||
| export const InputBoxContext = createContext<InputBoxContextType | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was there a final decision on naming this package/components ? Even thinking DateInputBox is a fine name considering it aligns with the Date web api which applies to both date and time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We briefly discussed the name InputBox, but it was never finalized, so I'm not opposed to changing the name.
As for DateInputBox, at first it made me think of only DatePicker because DateInputBox is a component within DatePicker, but, as you've just mentioned, the API aligns with both date and time, so this name makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever we choose, I would prefer to update the component name after the chain of PRs has been merged. I can open a PR just for the name change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, that sounds fine if that's easier than doing it now. Can we make a ticket to capture that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…utBoxTypes and remove deprecated InputSegment types
| export type { | ||
| InputSegmentChangeEvent, | ||
| InputSegmentChangeEventHandler, | ||
| } from './InputSegment.types'; | ||
| export type { SharedInputBoxTypes } from './types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason not to consolidate these all in a single shared.types.ts file? I see that InputSegment.types.ts types are used in InputBoxContext.types.ts as well, so it seems like a premature split to make shared/types/ subdir. Why not just packages/input-box/src/shared.types.ts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was mainly just to make things a little more organized. If it's overkill, I can switch to shared.types.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
…imports in InputBoxContext
✍️ Proposed changes
This PR is the first PR in a series of PRs that adds the new
InputBoxpackage and integrates it into the existingDatePickerpackage:InputSegment#3293InputBox#3285InputBoxcomponent #3286This PR adds
InputBoxContext, which will be used internally withinInputBoxandInputSegment.🎟 Jira ticket: LG-5504
✅ Checklist
For new components
🧪 How to test changes